Remove `DefId` from `EarlyParamRegion` #125468

BoxyUwU · 2024-05-23T23:56:56Z

Currently we represent usages of Region parameters via the ReEarlyParam or ReLateParam variants. The ReEarlyParam is effectively equivalent to TyKind::Param and ConstKind::Param (i.e. it stores a Symbol and a u32 index) however it also stores a DefId for the definition of the lifetime parameter.

This was used in roughly two places:

Borrowck diagnostics instead of threading the appropriate body_id down to relevant locations. Interestingly there were already some places that had to pass down a DefId manually.
Some opaque type checking logic was using the DefId field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a DefId manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the DefId around everywhere I previously tried to bundle it in with TypeErrCtxt but ran into issues with some call sites of infcx.err_ctxt being unable to provide a DefId, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of EarlyParamRegion from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of RegionKind over 8 bytes are all because they contain a BoundRegionKind which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the RegionKind type from 24 bytes to 12 (and with clever bit packing we might be able to get it to 8 bytes). I am curious what the performance impact would be of removing interning of Region's if we ever manage to shrink RegionKind that much.

Sidenote: by removing the DefId the Debug output for Region has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])
Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])

r? @compiler-errors (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)

rustbot · 2024-05-23T23:57:04Z

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler/rustc_smir/src/rustc_smir/convert/ty.rs

compiler-errors

Generally very happy with this change 👍 ✨

compiler/rustc_middle/src/ty/context.rs

compiler/rustc_lint/src/impl_trait_overcaptures.rs

compiler/rustc_lint/src/builtin.rs

compiler/rustc_hir_analysis/src/check/wfcheck.rs

compiler/rustc_infer/src/infer/error_reporting/mod.rs

compiler/rustc_borrowck/src/diagnostics/bound_region_errors.rs

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/util.rs

compiler/rustc_infer/src/infer/error_reporting/note.rs

compiler-errors · 2024-05-24T00:16:23Z

@bors try @rust-timer queue

bors · 2024-05-24T00:17:33Z

⌛ Trying commit 5451787 with merge 85c5dfc...

compiler-errors · 2024-05-24T00:35:29Z

🤔 also would like to know why those tests no longer ICE. I might be able to investigate tomorrow if you end up not being able to find out why, just lemme kno

BoxyUwU · 2024-05-24T00:39:02Z

Oh god why does this make tests no longer ICE 😭

src/tools/clippy/clippy_lints/src/ptr.rs

compiler-errors · 2024-05-24T01:29:51Z

@bors try @rust-timer queue

bors · 2024-05-24T01:31:01Z

⌛ Trying commit 0946cb8 with merge 8210bd9...

bors · 2024-05-24T03:04:12Z

☀️ Try build successful - checks-actions
Build commit: 8210bd9 (8210bd94475d1cbf00c0dd7bfe0d48e98259bca1)

rust-timer · 2024-05-24T06:24:39Z

Finished benchmarking commit (8210bd9): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[4.1%, 4.1%]	1
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.2%]	43
Improvements ✅ (secondary)	-0.8%	[-1.3%, -0.3%]	24
All ❌✅ (primary)	-0.4%	[-0.9%, -0.2%]	43

Max RSS (memory usage)

Results (secondary -3.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-3.7%, -3.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -1.4%, secondary 0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	8.0%	[7.1%, 8.9%]	2
Improvements ✅ (primary)	-1.4%	[-2.2%, -0.9%]	8
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.0%]	5
All ❌✅ (primary)	-1.4%	[-2.2%, -0.9%]	8

Binary size

Results (primary -0.3%, secondary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-1.3%, -0.0%]	88
Improvements ✅ (secondary)	-0.9%	[-1.6%, -0.0%]	12
All ❌✅ (primary)	-0.3%	[-1.3%, -0.0%]	88

Bootstrap: 671.784s -> 671.214s (-0.08%)
Artifact size: 315.72 MiB -> 315.68 MiB (-0.02%)

compiler-errors · 2024-05-24T14:45:22Z

TODO: I owe Boxy a detailed explanation for the "regression" (tests that ICE -> pass) which I'll have by the end of my day here hopefully. Then we can land this 🎉

compiler-errors · 2024-05-25T15:30:59Z

thinking emoji

@bors retry

bors · 2024-05-25T17:42:11Z

⌛ Testing commit ed8e436 with merge c154f01...

rust-log-analyzer · 2024-05-25T17:54:51Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

error: failed retrieving file 'mingw-w64-x86_64-gdb-14.1-1-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed retrieving file 'mingw-w64-x86_64-crt-git-11.0.0.r547.g4c8123efb-1-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed retrieving file 'mingw-w64-x86_64-libgccjit-13.2.0-3-any.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
warning: too many errors from mirror.msys2.org, skipping for the remainder of this transaction
error: failed retrieving file 'mingw-w64-x86_64-python-3.11.7-1-any.pkg.tar.zst.sig' from mirror.umd.edu : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
error: failed to commit transaction (unexpected error)
 mingw-w64-x86_64-gcc-13.2.0-3-any downloading...
 mingw-w64-x86_64-python-3.11.7-1-any downloading...
 mingw-w64-x86_64-gcc-ada-13.2.0-3-any downloading...

bors · 2024-05-25T17:55:30Z

💔 Test failed - checks-actions

BoxyUwU · 2024-05-25T19:23:50Z

CI is broken, see this zulip thread. I guess we should just wait a little bit...

@bors r-

compiler-errors · 2024-05-26T19:07:25Z

@bors r+

bors · 2024-05-26T19:07:28Z

📌 Commit ed8e436 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-05-26T19:07:29Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

BoxyUwU · 2024-05-26T19:10:28Z

5th times the charm lol :")

bors · 2024-05-27T06:37:01Z

⌛ Testing commit ed8e436 with merge fec98b3...

bors · 2024-05-27T08:43:58Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing fec98b3 to master...

rust-timer · 2024-05-27T10:45:48Z

Finished benchmarking commit (fec98b3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.2%]	48
Improvements ✅ (secondary)	-0.8%	[-2.0%, -0.3%]	19
All ❌✅ (primary)	-0.4%	[-0.9%, -0.2%]	48

Max RSS (memory usage)

Results (primary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-2.1%, -0.8%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-2.1%, 1.1%]	6

Cycles

Results (primary -1.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.4%	[-1.6%, -1.3%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.4%	[-1.6%, -1.3%]	5

Binary size

Results (primary -0.3%, secondary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-1.3%, -0.0%]	80
Improvements ✅ (secondary)	-0.9%	[-1.6%, -0.0%]	12
All ❌✅ (primary)	-0.3%	[-1.3%, -0.0%]	80

Bootstrap: 669.538s -> 668.124s (-0.21%)
Artifact size: 315.56 MiB -> 315.55 MiB (-0.00%)

rustbot assigned compiler-errors May 23, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 23, 2024

BoxyUwU commented May 23, 2024

View reviewed changes

compiler/rustc_smir/src/rustc_smir/convert/ty.rs Show resolved Hide resolved

BoxyUwU changed the title ~~Remove DefId from EarlyRegionParam~~ Remove DefId from EarlyParamRegion May 24, 2024

BoxyUwU force-pushed the remove_defid_from_regionparam branch from eafea35 to 5451787 Compare May 24, 2024 00:13

compiler-errors reviewed May 24, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 24, 2024

This comment has been minimized.

Sign in to view

BoxyUwU force-pushed the remove_defid_from_regionparam branch from 5451787 to 0946cb8 Compare May 24, 2024 00:58

This comment has been minimized.

Sign in to view

flip1995 reviewed May 24, 2024

View reviewed changes

src/tools/clippy/clippy_lints/src/ptr.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 24, 2024

oli-obk approved these changes May 24, 2024

View reviewed changes

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 25, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2024

bors merged commit fec98b3 into rust-lang:master May 27, 2024

rustbot added this to the 1.80.0 milestone May 27, 2024

bors mentioned this pull request May 27, 2024

Fix unnamed lifetime spans #125234

Closed

rustbot removed the perf-regression Performance regression. label May 27, 2024

Uh oh!

Remove DefId from EarlyParamRegion #125468

Remove DefId from EarlyParamRegion #125468

Uh oh!

Conversation

BoxyUwU commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 23, 2024

Uh oh!

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

compiler-errors commented May 24, 2024

Uh oh!

This comment has been minimized.

bors commented May 24, 2024

Uh oh!

This comment has been minimized.

compiler-errors commented May 24, 2024

Uh oh!

BoxyUwU commented May 24, 2024

Uh oh!

This comment has been minimized.

Uh oh!

compiler-errors commented May 24, 2024

Uh oh!

This comment has been minimized.

bors commented May 24, 2024

Uh oh!

bors commented May 24, 2024

Uh oh!

This comment has been minimized.

rust-timer commented May 24, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

compiler-errors commented May 24, 2024

Uh oh!

compiler-errors commented May 25, 2024

Uh oh!

bors commented May 25, 2024

Uh oh!

rust-log-analyzer commented May 25, 2024

Uh oh!

bors commented May 25, 2024

Uh oh!

BoxyUwU commented May 25, 2024

Uh oh!

compiler-errors commented May 26, 2024

Uh oh!

bors commented May 26, 2024

Uh oh!

bors commented May 26, 2024

Uh oh!

BoxyUwU commented May 26, 2024

Uh oh!

bors commented May 27, 2024

Uh oh!

bors commented May 27, 2024

Uh oh!

rust-timer commented May 27, 2024

Overall result: ✅ improvements - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Remove `DefId` from `EarlyParamRegion` #125468

Remove `DefId` from `EarlyParamRegion` #125468

BoxyUwU commented May 23, 2024 •

edited

Loading